Skip to content

Comments

[dotnet] [bidi] Make ContinueWithAuthCommand closer to spec (breaking change)#15545

Merged
nvborisenko merged 3 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-auth
Apr 3, 2025
Merged

[dotnet] [bidi] Make ContinueWithAuthCommand closer to spec (breaking change)#15545
nvborisenko merged 3 commits intoSeleniumHQ:trunkfrom
nvborisenko:bidi-auth

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Apr 1, 2025 •
edited by qodo-code-review bot
Loading

User description

Motivation and Context

Revisited ContinueWithAuthCommand to be closer with spec, fixes #15539.
Also avoided nested types, opening the doors for static factory.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests

Description

  • Refactored ContinueWithAuth method to align with BiDi spec.

  • Introduced new classes for clearer type differentiation.

  • Updated method overloads for better usability and clarity.

  • Adjusted test cases to validate the refactored implementation.

Changes walkthrough 📝

Relevant files
Enhancement
BiDiJsonSerializerContext.cs
Simplified serialization context for network commands       

dotnet/src/webdriver/BiDi/Communication/Json/BiDiJsonSerializerContext.cs

  • Removed unused JsonSerializable type for Default action.
  • Simplified serialization context for network commands.
    		• +0/-1     
    ContinueWithAuthCommand.cs
    Refactored `ContinueWithAuthCommand` with clearer abstractions

    dotnet/src/webdriver/BiDi/Modules/Network/ContinueWithAuthCommand.cs

  • Replaced nested types with standalone records for clarity.
  • Introduced ContinueWithAuthNoCredentials abstraction.
  • Added specific records for default and cancel actions.
  • Updated options classes to match new record structure.
    		• +16/-11 
    NetworkModule.cs
    Updated `ContinueWithAuthAsync` method overloads                 

    dotnet/src/webdriver/BiDi/Modules/Network/NetworkModule.cs

  • Updated ContinueWithAuthAsync method overloads.
  • Adjusted parameter types to match new record structure.
  • Improved method naming for better alignment with spec.
    		• +6/-6     
    Request.cs
    Adjusted `Request` methods for refactored auth handling   

    dotnet/src/webdriver/BiDi/Modules/Network/Request.cs

  • Updated ContinueWithAuthAsync calls to use new options classes.
  • Improved method signatures for consistency with refactored commands.
    		• +3/-3     
    Tests
    NetworkTest.cs
    Updated tests for refactored `ContinueWithAuth` implementation

    dotnet/test/common/BiDi/Network/NetworkTest.cs

  • Updated test cases to use new options classes.
  • Validated ContinueWithAuth behavior for all scenarios.
  • Removed outdated comments for better clarity.
    		• +2/-3     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 1, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    15539 - Fully compliant

    Compliant requirements:

    • Rework ContinueWithAuth method to align with BiDi spec
    • Apply method overloading for different authentication actions
    • Support the three actions defined in spec: "provideCredentials", "default", and "cancel"
    • Improve code structure to be closer to the spec definition

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Empty Test Block

    The test method for default credentials has an empty code block in the event handler. This might be a mistake as the ContinueWithAuthAsync call is present but commented out in the old code and added back in the new code.

     
    await using var intercept = await bidi.Network.InterceptAuthAsync(async e =>
{
    await e.Request.Request.ContinueWithAuthAsync(new ContinueWithAuthDefaultCredentialsOptions());
});

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Apr 1, 2025 •
    edited
    Loading

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add missing JSON property annotation

    The property name for Credentials doesn't match the expected JSON property name.
    According to the removed code, the property should be serialized as
    "credentials" but the new implementation doesn't specify this, which could cause
    serialization issues.

    dotnet/src/webdriver/BiDi/Modules/Network/ContinueWithAuthCommand.cs [34]

    -internal record ContinueWithAuthCredentials(Request Request, AuthCredentials Credentials) : ContinueWithAuthParameters(Request);
+internal record ContinueWithAuthCredentials(Request Request, [property: JsonPropertyName("credentials")] AuthCredentials Credentials) : ContinueWithAuthParameters(Request);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical issue where the JSON property name annotation for the Credentials property was lost during refactoring. Without this annotation, the serialized JSON would use "Credentials" instead of "credentials", breaking API compatibility.

    		High
    Fix options type casting

    The method is passing options directly to ExecuteCommandAsync, but the options
    parameter type has changed from ContinueWithAuthOptions to the more specific
    ContinueWithAuthCredentialsOptions. This could cause issues if the base command
    execution logic expects the base options type.

    dotnet/src/webdriver/BiDi/Modules/Network/NetworkModule.cs [107-110]

     internal async Task ContinueWithAuthAsync(Request request, AuthCredentials credentials, ContinueWithAuthCredentialsOptions? options = null)
 {
-    await Broker.ExecuteCommandAsync(new ContinueWithAuthCommand(new ContinueWithAuthCredentials(request, credentials)), options).ConfigureAwait(false);
+    await Broker.ExecuteCommandAsync(new ContinueWithAuthCommand(new ContinueWithAuthCredentials(request, credentials)), options as ContinueWithAuthOptions).ConfigureAwait(false);
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion addresses a potential runtime issue where the more specific options type might not be compatible with what the ExecuteCommandAsync method expects. Casting to the base type ensures type compatibility while maintaining the improved type safety of the refactored code.

    		Medium
    Learned
    best practice
    Add null parameter validation to prevent potential NullReferenceExceptions

    The method accepts potentially null parameters (request and credentials) but
    doesn't validate them before use. Add null checks using
    ArgumentNullException.ThrowIfNull() to prevent potential NullReferenceExceptions
    when these parameters are accessed.

    dotnet/src/webdriver/BiDi/Modules/Network/NetworkModule.cs [107-110]

     internal async Task ContinueWithAuthAsync(Request request, AuthCredentials credentials, ContinueWithAuthCredentialsOptions? options = null)
 {
+    ArgumentNullException.ThrowIfNull(request, nameof(request));
+    ArgumentNullException.ThrowIfNull(credentials, nameof(credentials));
     await Broker.ExecuteCommandAsync(new ContinueWithAuthCommand(new ContinueWithAuthCredentials(request, credentials)), options).ConfigureAwait(false);
 }
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    		Low
    • Update

    RenderMichael
    RenderMichael approved these changes Apr 2, 2025
    View reviewed changes
    Copy link
    Contributor

    @RenderMichael RenderMichael left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    I like the pattern!

    @nvborisenko nvborisenko changed the title [dotnet] [bidi] Make ContinueWithAuthCommand closer to spec [dotnet] [bidi] Make ContinueWithAuthCommand closer to spec (breaking change) Apr 2, 2025
    nvborisenko and others added 2 commits April 3, 2025 12:50
    @nvborisenko @RenderMichael
    New ending line
    35456bd 
    Co-authored-by: Michael Render <render.michael@gmail.com>
    @nvborisenko
    Merge branch 'trunk' into bidi-auth
    6d40d3c
    @nvborisenko nvborisenko merged commit 57bf403 into SeleniumHQ:trunk Apr 3, 2025
    10 checks passed
    @nvborisenko nvborisenko deleted the bidi-auth branch April 3, 2025 13:42
    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    This was referenced Dec 21, 2025
    Open
    Closed
    Closed
    Closed
    This was referenced Jan 19, 2026
    Closed
    Closed
    Closed
    Merged
    This was referenced Feb 16, 2026
    Closed
    Closed
    This was referenced Feb 23, 2026
    Closed
    Open
    Open
    Open
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @RenderMichael RenderMichael RenderMichael approved these changes

    Assignees

    No one assigned

    Labels

    Review effort 2/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: [dotnet] [bidi] Rework ContinueWithAuth method

    2 participants

    @nvborisenko @RenderMichael